State that payloadId should be unique for each PayloadAttributes instance
#401
Conversation
|
I have changed the original statement it was missing the new build process to be set in motion every time when new |
src/engine/paris.md
Outdated
| @@ -136,6 +136,8 @@ The payload build process is specified as follows: | |||
|
|
|||
| 4. Client software **SHOULD** stop the updating process when either a call to `engine_getPayload` with the build process's `payloadId` is made or [`SECONDS_PER_SLOT`](https://github.com/ethereum/consensus-specs/blob/dev/specs/phase0/beacon-chain.md#time-parameters-1) (12s in the Mainnet configuration) have passed since the point in time identified by the `timestamp` parameter. | |||
|
|
|||
| 5. Client software **MUST** initiate new payload build process for every distinct `PayloadAttributes` instance. A `payloadId` value **MUST** uniquely identify every new build process. | |||
There was a problem hiding this comment.
Does distinct mean shallow or deep equality here? I.e. does the payload ID need to be different when the payload attributes match another set of payload attributes fully, but from a different RPC request?
We previously had "hashing to payload ID", but that was removed here: 37c8852
And at one point I think it was also defined as a randomized ID.
With each update that changes the payload attributes definition, the hashing to ID makes things quite difficult: the hashing has to be updated to avoid simple collisions between the new attributes (same basic request, but different additional attribute would hash to the same payload ID if not updated). Customizing payload attributes for EIPs / extensions also results in inconsistent hashing.
And when a collision is hit, the 2nd payload attributes attempt with different unhashed attribute cannot happen because of the first attempt. With randomized ID this is very rare, but with broken hashing it can happen. Some kind of unique id / incremental ID without collisions would be even better.
So I'm in favor of an ID not hard-wired to the attributes contents, and unique per build-process instantiating RPC request.
There was a problem hiding this comment.
Does
distinctmean shallow or deep equality here?
Whenever new payload attributes differ to the previous ones in a set of fields or field values, I suppose it is deep equality. And I can see potential confusion because of the "instance" word in the statement, probably reword to:
| 5. Client software **MUST** initiate new payload build process for every distinct `PayloadAttributes` instance. A `payloadId` value **MUST** uniquely identify every new build process. | |
| 5. Client software **MUST** initiate new payload build process for every new `PayloadAttributes` object, i.e. whenever a set of fields or field values of the new object is different to the previously sent one. A `payloadId` value **MUST** uniquely identify every new build process. |
Some kind of unique id / incremental ID without collisions would be even better.
I entirely agree with that. So, the ID must be different for two different build processes and ID value shouldn't be derived from payload attributes.
unique per build-process instantiating RPC request
Does it mean that every time the call to fcU with the same attributes will be responded with new payloadId while the build process remains untouched? So, there are multiple payloadId identifying the same build process. Why would this be helpful?
There was a problem hiding this comment.
Agree that random UID would mean easier maintenance and less scope for bugs.
There are cases where the CL replays the FCU so using a random id each time would prevent ELs from ignoring these replays, trigger the EL to start a new block building activity and worst case may cause late/missed proposals.
I think @potuz mentioned why they sometimes replay the FCU in a recent discussion.
There was a problem hiding this comment.
I re-phrased the original statement, please, take a look
|
From a discussion in discord#consensus-dev: CL sometimes may replay In a conversation with @protolambda and in discord#interop we concluded that using a prefix of hash function output to compute |
Adds a statement that
payloadIdvalue returned EL client software should be different for two distinctPayloadAttributesobjects